-
-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Greg/validator/rpc #185
Greg/validator/rpc #185
Conversation
Codecov Report
@@ Coverage Diff @@
## master #185 +/- ##
==========================================
+ Coverage 31.85% 31.93% +0.08%
==========================================
Files 46 50 +4
Lines 744 811 +67
Branches 64 70 +6
==========================================
+ Hits 237 259 +22
- Misses 507 552 +45 |
src/rpc/api/modules/validator.ts
Outdated
return {} as BeaconBlock; | ||
} | ||
|
||
public async produceAttestation(slot: Slot, shard: Shard): Promise<IndexedAttestation> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want to return an AttestationData
, see getAttestationData of the BeaconApi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure? IndexedAttestation
contains AttestationData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, now I see the issue, opPool.receiveAttestaion
looks for AttestationData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm looking at https://github.com/ethereum/eth2.0-specs/blob/dev/specs/validator/0_beacon-chain-validator.md#attestations-1
It seems like the validator is supposed to get an AttestationData and turn it into an Attestation, and then publish that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm ok I see what you are saying -- So the beacon chain should send the validator attestationData
and then the validator turns it into an IndexedAttestation
?
src/rpc/api/modules/validator.ts
Outdated
return {} as BeaconBlock; | ||
} | ||
|
||
public async produceAttestation(slot: Slot, shard: Shard): Promise<AttestationData> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should probably call this produceAttestationData
just to be more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See i think we should probably go back to IndexedAttestation
based on the gitter conversations its a lot of extra work to just send the data..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 looks good
Loosely implemented off of ethereum/consensus-specs#1011
Also includes a refactor of the
rpc/api
section. Added two directoriesmodules
andinterfaces
where we can keep our classes that contain the logic.What this doesn't include is name spacing of the api methods. I'll make an issue and do that next.